Skip to content

fix: add modelgen support for granular read ops#537

Closed
AaronZyLee wants to merge 2 commits intomainfrom
granular-read
Closed

fix: add modelgen support for granular read ops#537
AaronZyLee wants to merge 2 commits intomainfrom
granular-read

Conversation

@AaronZyLee
Copy link
Contributor

@AaronZyLee AaronZyLee commented Jan 31, 2023 •
edited
Loading

Description of changes

Added granular modelgen support for Java
For the other libraries, the generated code has no problem and only library support is needed

Issue #, if available

aws-amplify/amplify-flutter#2526

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • Breaking changes to existing customers are released behind a feature flag or major version update
  • Changes are tested using sample applications for all relevant platforms (iOS/android/flutter/Javascript) that use the feature added/modified

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AaronZyLee AaronZyLee requested a review from a team as a code owner January 31, 2023 00:39
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.69%. Comparing base (c6c7b7a) to head (a54ddaa).
Report is 428 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #537   +/-   ##
=======================================
  Coverage   85.69%   85.69%           
=======================================
  Files         148      148           
  Lines        7380     7380           
  Branches     1962     1962           
=======================================
  Hits         6324     6324           
  Misses        959      959           
  Partials       97       97

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

phani-srikar
phani-srikar suggested changes Feb 3, 2023
View reviewed changes
packages/appsync-modelgen-plugin/src/visitors/appsync-java-visitor.ts
list: 'ModelOperation.LIST',
sync: 'ModelOperation.SYNC',
listen: 'ModelOperation.LISTEN',
search: 'ModelOperation.SEARCH',
Copy link
Contributor

@phani-srikar phani-srikar Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we changing the behavior for only appsync-java-visitor or all other visitors as well? I assume this generateAuthRules method is common to all visitors? If that's the case, can we move this to a common visitor? Thanks

Copy link
Contributor Author

@AaronZyLee AaronZyLee Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for java visitor only. It maps the input operation to the library-defined enum value. If not added here, the code in java will be generated as empty string.
For the other code generators, these ops will be simply parsed and generated in the final code and do not need additional mapping. You can see this in the snapshot changes for other visitors

Copy link
Contributor Author

@AaronZyLee AaronZyLee Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpilch dpilch closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phani-srikar phani-srikar Awaiting requested review from phani-srikar

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AaronZyLee @codecov-commenter @phani-srikar @dpilch

Comments